-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Suggest cfg(false) instead of cfg(FALSE)
#149792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Some changes occurred in check-cfg diagnostics cc @Urgau |
|
LGTM. r=me once CI passes. @bors delegate+ |
|
✌️ @clubby789, you can now approve this pull request! If @joshtriplett told you to " |
This comment has been minimized.
This comment has been minimized.
|
hm, have you seen @scrabsha's work on uppercase/lowercase suggestions in the parser? I think it'd be cool if we could suggest against all mis-capitalizations here and maybe do that in a general enough way that all attribute parsers can benefit from it instead of a cfg-specific lint. At least, could you take a quick look if we can re-use the parsing infra for it? I like it when the error messages are consistent :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that doesn't help let me know and I'll approve this version. The change is a good one.
|
Reminder, once the PR becomes ready for a review, use |
|
@bors delegate- |
|
I've spent a while looking into this and I'm not sure it makes sense to factor out the exact machinery from parsing, especially as the attr parsing machinery is some distance from the check-cfg lint. |
|
ok, but this wouldn't trigger on any other mis-capitalization of false. I think it's reasonable to at least also detect |
|
@rustbot author |
|
@rustbot ready |
|
one last thing, could you explain that replacing it with false doesn't mean the cfg named false, but like, it literally means false. like, with this it seems like false is just another defined cfg name. Something like:
and similar for true. |
| //~| HELP: write it in lowercase (notice the capitalization) | ||
| pub fn b() {} | ||
|
|
||
| #[cfg(r#false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to have a test for r#False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently raises the suggestion on r#False. I switched to just checking if the source code has r# to prevent this.
|
Extended to support |
|
I like it. Thanks @clubby789 ! Sorry I kept having smol things to fix. @bors r+ rollup |
Split from #149791
cc @Urgau